Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Configurable group ids region names #55

Merged
merged 7 commits into from
Nov 24, 2023
Merged

Configurable group ids region names #55

merged 7 commits into from
Nov 24, 2023

Conversation

mgeplf
Copy link
Collaborator

@mgeplf mgeplf commented Oct 5, 2023

No description provided.

mgeplf added 3 commits October 5, 2023 16:20
@mgeplf
put cleanup_rest logic into place it's used
da5e02d
@mgeplf
add golden tests for get_group_ids, _get_group_names
ddb48dc
@mgeplf
added group_ids_config.json
fa19aa9 
* so that ids can be specified at runtime for densities
* plumbed it through the rest of the code
@mgeplf mgeplf marked this pull request as draft October 5, 2023 14:22
@codecov-commenter
Copy link

codecov-commenter commented Oct 6, 2023 •
edited
Loading

Codecov Report

All modified lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@981e70a). Click here to learn what that means.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main      #55   +/-   ##
=======================================
  Coverage        ?   97.76%           
=======================================
  Files           ?       22           
  Lines           ?     1429           
  Branches        ?        0           
=======================================
  Hits            ?     1397           
  Misses          ?       32           
  Partials        ?        0
Flag Coverage Δ
pytest 97.76% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mgeplf mgeplf force-pushed the group-ids-region-names branch from 9911901 to 57070f3 Compare October 6, 2023 07:05
@mgeplf mgeplf marked this pull request as ready for review October 6, 2023 12:38
@mgeplf mgeplf force-pushed the group-ids-region-names branch from f8d862e to 9b324fe Compare October 6, 2023 12:47
@mgeplf
Copy link
Collaborator Author

mgeplf commented Oct 6, 2023

@lecriste; this is ready to try - please pass the config with the updates for the steps that require changes, and let me know if it it works

@mgeplf mgeplf mentioned this pull request Oct 6, 2023
Closed
@lecriste
Copy link
Contributor

I tested the four steps requiring the config and I confirm I got the expected result.

lecriste
lecriste requested changes Nov 21, 2023
View reviewed changes
Copy link
Contributor

@lecriste lecriste left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main Readme should be updated with the new --group-ids-config-path argument for the four CLIs,

@mgeplf
Copy link
Collaborator Author

mgeplf commented Nov 23, 2023

The main Readme should be updated with the new --group-ids-config-path argument for the four CLIs

Since it's not a required argument, I think it's ok to allow people to discover it themselves if they need the additional configurability.

@lecriste
Copy link
Contributor

The main Readme should be updated with the new --group-ids-config-path argument for the four CLIs

Since it's not a required argument, I think it's ok to allow people to discover it themselves if they need the additional configurability.

How can one discover it?

@mgeplf mgeplf force-pushed the group-ids-region-names branch from 9b324fe to e9b6fdc Compare November 23, 2023 12:50
@mgeplf
Copy link
Collaborator Author

mgeplf commented Nov 23, 2023

How can one discover it?

Using the --help that's provided with all the commands and subcommands:

atlas-densities cell-densities glia-cell-densities --help

@lecriste
Copy link
Contributor

How can one discover it?

Using the --help that's provided with all the commands and subcommands:

atlas-densities cell-densities glia-cell-densities --help

I find it quite hidden.
Also, it is a required argument for the functions, it's just the click option that provides a default value.

@mgeplf
Copy link
Collaborator Author

mgeplf commented Nov 23, 2023

I find it quite hidden.

It's a tradeoff between having a readme that is quite large and comprehensive vs. something that can be run and tested for people to see what is available - if they need more functionality, they should read the --help options, IMO.

Also, it is a required argument for the functions, it's just the click option that provides a default value.

The README is documenting the CLI, so the fact that it's required for the function isn't really an issue, I would say.

@mgeplf mgeplf requested a review from lecriste November 24, 2023 14:02
lecriste
lecriste approved these changes Nov 24, 2023
View reviewed changes
Copy link
Contributor

@lecriste lecriste left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would mention the new config CLIs argument in the main ReadMe.

@mgeplf mgeplf merged commit e8cf882 into main Nov 24, 2023
5 checks passed
@mgeplf mgeplf deleted the group-ids-region-names branch November 24, 2023 15:37
lecriste added a commit to BlueBrain/bbp-atlas-pipeline that referenced this pull request Sep 10, 2024
@lecriste
Update atlas-densities to BlueBrain/atlas-densities#55
65c19b6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lecriste lecriste lecriste approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mgeplf @codecov-commenter @lecriste